Repository navigation
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-169/ This page is automatically updated on each push to this PR. |
bd66c1f to
3df2d8e
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-169/ This page is automatically updated on each push to this PR. |
1 similar comment
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-169/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues remain for the shared two-factor disabling service.
No open issues 🎉
What changed in this PR
Adds a shared disableTwoFactor service method for the planned MFA reset command and profile/admin controls, building on the existing MFA infrastructure.
Changes:
- Defines the actor-based password requirement and caller-owned
settings_changedaudit contract. - Disables 2FA, deletes recovery codes, and revokes trusted devices in one transaction, followed by best-effort audit logging.
- Adds unit and database integration tests covering actor rules, state cleanup, and audit failure handling.
| File | Description |
|---|---|
tests/unit/RecoveryCodeServiceDisableTwoFactorTest.php |
Tests password rules, disable operations, and audit failure handling. |
tests/DisableTwoFactorIntegrationTest.php |
Verifies database cleanup, audit events, and unchanged state after an incorrect password. |
app/Services/Auth/RecoveryCodeService.php |
Implements transactional disabling and trusted-device revocation. |
app/Services/Auth/IRecoveryCodeService.php |
Declares the method and documents password, audit, and group-enforcement behavior. |
Review effort: Balanced
| { | ||
| // self-service requires the current password; console and admin | ||
| // operations do not (see IRecoveryCodeService::disableTwoFactor) | ||
| if (!is_null($actor) && $actor->getId() === $user->getId() |
There was a problem hiding this comment.
@romanetar A self-enrolled user without a password (social-only account) can never disable their own 2FA through this path: enableTwoFactor does not ask for a password, but the self-service branch here requires checkPassword(), and User::checkPassword() returns false when the password is empty.
No caller uses the self-service branch yet, so nothing breaks today, but the profile toggle of ticket 14 (https://app.clickup.com/t/86bc64n02) will be that caller.
Suggested fix: document in the IRecoveryCodeService::disableTwoFactor PHPDoc that, for self-service with !$user->hasPasswordSet(), the caller must require another proof of identity. Under SDS rev 2.5 that is a recent 2FA challenge (MFAConstants::SESSION_KEY_COMPLETED_AT). Add a unit test for the passwordless self-service case so the behaviour is pinned.
| // a single transaction() call: nesting another one (e.g. through the audit | ||
| // service) inside it would let an inner failure close the entity manager | ||
| // under this still-running outer transaction | ||
| $this->tx_service->transaction(function () use ($user) { |
There was a problem hiding this comment.
@romanetar Once the users-row lock from SDS rev 2.6 lands (https://github.com/fntechgit/ftn-docsnsklz/pull/179, §3.5; ticket https://app.clickup.com/t/86bcy7rv7, task 3), this transaction should take the same PESSIMISTIC_WRITE lock on the user's row first. It deletes recovery codes too, and without the lock an admin reset racing a user regenerate (READ_COMMITTED, bulk DELETE with no lock) can let the regenerate insert after the delete, leaving a "reset" user with a fresh batch.
No change needed in this PR: the lock does not exist yet. It is tracked in task 9 of the recovery-code enrollment ticket, next to resetting recovery_codes_acknowledged_at here.
Onboards every user of the groups in two_factor.enforced_groups: generates
a recovery code batch, emails the user that MFA is now required (no codes in
the mail or the console output) and records an enrollment_changed audit event
with {"source": "enforce-admin-2fa"}.
Users that already have unused recovery codes are skipped so a saved batch is
never invalidated. Supports --dry-run, chunked queries and processed /
skipped / error counts.
It deliberately does not set two_factor_enabled / two_factor_method /
two_factor_enforced_at: enforcement is derived from group membership by
User::shouldRequire2FA().
Shared service method that disables 2FA for a user: clears two_factor_enabled and two_factor_enforced_at, deletes every recovery code and revokes every trusted device in a single transaction, then records a device_revoked audit event best-effort. The actor decides the password rule: null (console) and another user (admin) need no password, the user acting on their own account must provide the current one. The settings_changed audit event is left to the caller, which knows the context (reason, actor, operator). Group-enforced users are still challenged at login afterwards, as enforcement is derived from group membership.
3df2d8e to
fd29e5c
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-169/ This page is automatically updated on each push to this PR. |
1 similar comment
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-169/ This page is automatically updated on each push to this PR. |
| $user->disable2FA(); | ||
| $this->user_repository->add($user, false); | ||
| $this->repository->deleteAllForUser($user); | ||
| $this->trusted_device_repository->revokeAllForUser($user); |
There was a problem hiding this comment.
@romanetar Heads-up from the #160 review: #160 removes IUserTrustedDeviceRepository::revokeAllForUser(), so once both PRs merge this call fails with "Call to undefined method" (git merges the two branches without conflicts). See the comment on IUserTrustedDeviceRepository::getByIdAndUser in #160 for the proposed shared bulk revoke and the per-device device_revoked audit semantics that #160 established.
ref: https://app.clickup.com/t/86ba2zp7r
Stacked on #166 (base:
feat/mfa-admin-enforcement-and-reset-commands), which is stacked on #125. Merge bottom-up: #125, #166, then this one.Summary
Provisional implementation of the shared disable/reset service method that ClickUp ticket 12 (
idp:reset-2fa) needs and ticket 14 (profile toggle and admin CRUD) will reuse. The ticket allows implementing it in ticket 12 when ticket 14 has not landed, with this exact contract:IRecoveryCodeService::disableTwoFactor(User $user, ?string $currentPassword, ?User $actor): voiddisable2FA()(clearstwo_factor_enabledandtwo_factor_enforced_at), deletes every recovery code, and revokes every trusted device (is_revoked = 1, rows are kept).device_revokedaudit event, best-effort: a failure there does not fail the operation.$actordecides the password rule:null(console, e.g.idp:reset-2fa): no password.$currentPasswordmust match, otherwiseValidationExceptionand nothing changes.settings_changed: the caller knows the context (reason, actor, operator) and emits it, so there is exactly one row.User::shouldRequire2FA()).Things to know
DeviceTrustService::removeTrustedDevicesas the ticket suggests: that method's audit call opens its own transaction, and nesting transactions can close the entity manager if the inner one fails (the existing code inRecoveryCodeServicewarns about this). It usesIUserTrustedDeviceRepository::revokeAllForUserinside the same transaction instead, which is whatremoveTrustedDevicesdoes for the DB part.RecoveryCodeServicegets a new constructor dependency (IUserTrustedDeviceRepository). Nothing instantiates it withnew; the container resolves it.disableTwoFactor, the two need to be reconciled; ticket 14 can adopt this one directly.Test plan
tests/unit/RecoveryCodeServiceDisableTwoFactorTest.php: 6 tests (console, admin, self-service with correct / wrong / missing password, audit failure).tests/DisableTwoFactorIntegrationTest.php: 2 tests against the DB (everything cleared, 2 devices revoked, 1device_revokedrow and nosettings_changed; wrong password leaves state untouched).tests/RecoveryCodeRegenerationTest.phpalready fails on the base branch (Class "Strategies\MFA\MFAChallengeStrategyFactory" not found), unrelated to this change.